Session ID and constructor #400

jburel · 2024-02-29T15:13:32Z

Specifying a client as a parameter leads to strange behaviour when attempting to access session or connect
Looking at the constructor of the BlitzGateway, the connected flag is set even if a client without session is specified.

This leads to error like

omero.RemovedSessionException: exception ::omero::RemovedSessionException
{
serverStackTrace = ome.conditions.RemovedSessionException: No context for
at ome.services.sessions.state.SessionCache.getDataNullOrThrowOnTimeout(SessionCache.java:432)
at ome.services.sessions.state.SessionCache.getSessionContext(SessionCache.java:368)

See TheJacksonLaboratory/ezomero#100 for background

This PR:

Sets the host and port from the client if the client is passed a parameter
Sets the session and sessionID if a "truly" connected client is passed
Makes sure that the session is not closed if associated to passed client when connect is invoked. In that case a new client is created and the Gateway joins the session

To test:
Check that the sessionID is set.

client = omero.client(server)
client.createSession(user, password)
sid = client.getSessionId()
with BlitzGateway(client_obj=client) as conn:
    assert sid == conn.getSession().getUuid().val

Check that the session is not closed

client=omero.client(server)
client.createSession(user, password)
with BlitzGateway(client_obj=client) as conn:
    assert conn.connect(), "Session is closed"

Check you can connect after setting the identity

client=omero.client(server)
with BlitzGateway(client_obj=client) as conn:
   conn.setIdentity(user, password)
    assert conn.connect(), "New session cannot be created"

client=omero.client(server)
c=omero.client(server)
c.createSession(user1, password1)
with BlitzGateway(client_obj=client) as conn:
   assert Uuid=c.getSessionId(), "Session cannot be joined"

client=omero.client(server)
c=omero.client(server1)
c.createSession(user1, password1)
with BlitzGateway(client_obj=client) as conn:
   asserts Uuid != c.getSessionId(), "Different host, should not work"

Note that none of the code snippets work if the host is not retrieved from the specified client. In order to work the host parameter needs to be set

sbesson

I think omero-user-token and joinSession is a red herring. I am able to reproduce the initial issue when calling omero.gateway.BlitzGateway.getSession using the following minimal example

import omero
from omero.gateway import BlitzGateway
client=omero.client(server)
client.createSession(user, password)
with BlitzGateway(client_obj=client) as conn:
    conn.getUser()
    conn.getSession()

Minimally, the proposed change allows the session UUID to be retrieved dynamical and conn.getSession() works as expected in the scenario above.

This raises a few higher level API questions about using client objects directly which I could not answer by looking at the implementation or the documentation:

are there other APIs depending on the internal self._sessionUuid value and if should this internal field be updated as well in getSession()?
what is the expectation when passing a client object to the constructor of the BlitzGateway? Is the contract that the client should have an initialised session? Or is it possible to pass an empty client and use another API to create a session?
related to the previous point, I was surprised to see that
```
import omero
from omero.gateway import BlitzGateway
client=omero.client(server)
client.createSession(user, password)
with BlitzGateway(client_obj=client) as conn:
  conn.connect()
```
returns False although this has some relationship to the fact that _sessionUuid is set to None. Is this a valid workflow we should consider or is it expected that the user should never call connect() if a client object is passed?

jburel · 2024-03-01T09:07:32Z

I was thinking about similar issues last night and I agree that it requires some investigations

jburel · 2024-03-01T10:57:00Z

From the _init_ method

if self.c is None:
            self._resetOmeroClient()
        else:
            # if we already have client initialised, we can go ahead and create
            # our services.
            self._connected = True
            self._createProxies()
            self.SERVICE_OPTS = self.createServiceOptsDict()

This indicates that a connection is established so the client should have an established connection but nothing is set.
This seems to be contradicting the description of the method

DavidStirling · 2024-03-01T11:04:57Z

I'd also been looking into this and came to a similar conclusion.

Fetching the UUID in the getSession() function seems to fix this particular case, but I wonder if it may be worth instead checking for and setting _sessionUuid within the init function when a pre-existing client is passed in. This would also provide an opportunity to validate whether the client is actually set up/connected properly.

If my understanding is correct this would then allow users to join or rejoin the session by calling conn.connect() if anything goes wrong, which currently fails and/or tries to create a new session.

jburel · 2024-03-01T12:23:34Z

Digging a bit more into the problem
The reason that

with BlitzGateway(client_obj=client) as conn:
  conn.connect()

returns False
is due to the fact that information from the passed client are not taken into account
host when a new client is created (if session_id is None)
and username and password when the session is created

I use the following code snippet to validate

with BlitzGateway(client_obj=client, host=server) as conn:
    conn.setIdentity(user, password)
    print(conn.connect())

This will return True

If self._sessionUuid is not none, then a new client does not need to be created and the session id is used to "rejoin" setting the sessionUuidwill solve one problem but it will not solve the assumption made in_resetOmeroClient`` about the host if a client with host is specified.

jburel · 2024-03-01T15:25:30Z

Setting the sessionUuid from the client if available leads to other problems when
testing via

client=omero.client(server)
client.createSession(user, password)
with BlitzGateway(client_obj=client) as conn:
  conn.connect()

error

exception ::Glacier2::PermissionDeniedException
{
    reason = Internal error. Please contact your administrator:
 Wrapped Exception: (org.springframework.ldap.AuthenticationException):
[LDAP: error code 49 - INVALID_CREDENTIALS: Bind failed: ERR_229 Cannot authenticate user uid=admin,ou=system]; nested exception is javax.naming.AuthenticationException: [LDAP: error code 49 - INVALID_CREDENTIALS: Bind failed: ERR_229 Cannot authenticate user uid=admin,ou=system]
}

No idea about this ldap error since I am not using a LDAP user
so not there yet

jburel · 2024-03-01T16:46:46Z

client=omero.client(server)
client.createSession(user, password)
print(client.getSessionId())
c=omero.client(server)
c.joinSession(client.getSessionId())
print(c.getSessionId())

This works fine so something in the gateway is happening

jburel · 2024-03-01T17:09:01Z

I found the problem:
if the sessionUuid is set,
in the connect method

the client is deleted in _resetOmeroClient(self) so the session is no longer valid
leading to the error when attempting to join that session

sbesson

This might have created some possible regressions one of the gateway integration tests - see https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/374/testReport/OmeroPy.test.integration.gatewaytest.test_chmod/TestDefaultSetup/testAuthorCanEdit/

Also it would be useful to update the PR title and description to match the latest changes and possibly list the different connection/reconnection scenarios that should be tested

jburel · 2024-03-04T12:06:32Z

The regression is possibly due to the fact that the session is not discarded if it is coming from the client.
In that case the contract will have to be different
It seems that the "flexibility" of the constructor leads to the fact that some methods cannot be invoked depending on the constructor used.

jburel · 2024-03-04T12:17:04Z

I have updated the description with various scenarii I have tried

DavidStirling · 2024-03-04T12:46:31Z

src/omero/gateway/__init__.py

@@ -1525,14 +1525,24 @@ def __init__(self, username=None, passwd=None, client_obj=None, group=None,
        self.ice_config = [os.path.abspath(str(x)) for x in [_f for _f in self.ice_config if _f]]

        self.host = host
+        if self.c is not None:
+            self.host = self.c.getProperty("omero.host")


I could imagine an scenario where a user tries to connect to a server with the wrong client object, which would then cause some ambiguity over exactly which server Blitz is connected to.

Could I suggest that we check if self.host is already set to something different here, and if there's a mismatch throw an error?

We could add a check but i think people either specify the "client" or the host/user/password
Checking the host does not stop the connection to a wrong server. Using the "connect" specifying the sessionID could lead to that situation cf. one of the examples in the description and there is "false" value return

i think the gateway is too "flexible" leading to some very strange situations.

Should we also set the identity if a client with connection is specified?
the same will apply i.e. checking the value from client and via constructor.

Client with session specified, specify user name and password different from the client ones. Identity in gateway does not correspond to the identity in client

I agree that people should either specify client or host, but in the event that they don't I think it's safer to avoid silently overriding the user-supplied host parameter. Some of the autocompletion tools people now use do have a habit of trying to fill every argument. The user really should be reading the docs, but at the same time I don't think Gateway's Python docs are as helpful as they need to be.

You're probably right that Gateway's excessive flexibility here may be causing more problems than it solves. An alternative might be to validate the connection in the client object and outright reject calls with the other parameters specified - you either init with a connected client or supply credentials, no "mix and match".

There are other problems especially with the connect method since new clients get created (even if one was specified in the constructor).
So probably need to describe what we expect from each method moving forward

The more I look into it the more I notice points that require clarification.
The clone method for example will not work when a client with session is specified.
To make it works one should do

BlitzGateway(client_obj=client) conn.setIdentity(user, password)

We also lose info like useragent and userid in that case

will-moore · 2024-03-04T13:47:22Z

There is a test failing at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/lastCompletedBuild/testReport/OmeroPy.test.integration.gatewaytest.test_chmod/TestDefaultSetup/testAuthorCanEdit/
I can't be sure whether it is due to this PR - I haven't yet managed to get integration tests running on my local machine, so I can't test there. Since @jburel is away just now, he suggests I exclude to see if that fixes the test for now...

joshmoore

Few comments after a review triggered by the failure in https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/17/testReport/OmeroPy.test.integration.gatewaytest.test_chmod/TestDefaultSetup/testAuthorCanEdit/

src/omero/gateway/__init__.py

joshmoore · 2024-04-02T08:53:42Z

src/omero/gateway/__init__.py

        self.secure = secure
+        if self.c is not None:
+            self.secure = self.c.isSecure()


Not overly important, but should there be a mismatch test here?

I had a similar question while performing code review but this was marked as resolved. Was this addressed somewhere?

tested added there https://github.com/ome/openmicroscopy/pull/6381/files

My interpretation of @joshmoore's question was: what should happen if a gateway object is constructed with an initialized client object and secure={True,False} and the encryption state of the client as returned by self.c.isSecure() does not match the secure keyword?

jburel · 2024-04-05T20:04:15Z

@DavidStirling I have added integration tests for the proposed changes (all green)
Anything else you came across during your investigation?

src/omero/gateway/__init__.py

DavidStirling · 2024-04-11T07:16:41Z

LGTM, thanks

sbesson · 2024-04-11T08:19:04Z

src/omero/gateway/__init__.py

@@ -2029,7 +2049,7 @@ def isSecure(self):
        Returns 'True' if the underlying omero.clients.BaseClient is connected
        using SSL
        """
-        return hasattr(self.c, 'isSecure') and self.c.isSecure() or False


Isn't that modifying the contract of the API before a connection happens e.g. in a scenario like the following

>>> conn=BlitzGateway(username="test",passwd="test",host="localhost",secure=False) >>> conn.isSecure() True >>> conn.secure False

This raises another question in that case
When a client is not specified, a secure client is created regardless of the secure parameter specified. But when a client is specified, it can be secured or unsecured.

The initial connection will (and should) always use an encrypted client. But

omero-py/src/omero/gateway/__init__.py

Line 2074 in 4dc40d6

self.setSecure(self.secure)

in _createSession might create an unencrypted client depending on the state of secure:

>>> conn=BlitzGateway(username=user,passwd=password,host="localhost") >>> conn.connect() True >>> conn.isSecure() False >>> conn.close() >>> conn=BlitzGateway(username=user,passwd=password,host="localhost",secure=True) >>> conn.connect() True >>> conn.isSecure() True

i get that
but as I said the client specified could be insecured and in that case even if secure=True is passed
conn.isSecure() will be false.
Do we want in that case to enforce a "secure" client specified during init

So the scenario we are discussing is

conn=BlitzGateway(client_obj=insecure_client, secure=True) conn=BlitzGateway(client_obj=secure_client, secure=False)

Interestingly, part of the changes proposed here are to throw an exception on the following use cases

conn=BlitzGateway(client_obj=client, host=different_host) conn=BlitzGateway(client_obj=client, port=different_port)

My feeling would be to say secure should behave in the same way i.e. if the constructor receives conflicting information from its inputs, an exception should be sent back to the caller. Unlike host and port the default of secure is False. A possible (untested) solution would be to change the default to None to cater for this scenario (and internally default to False if no client object is passed).

src/omero/gateway/__init__.py

sbesson · 2024-04-11T08:23:39Z

src/omero/gateway/__init__.py

        self.secure = secure
+        if self.c is not None:
+            self.secure = self.c.isSecure()


I had a similar question while performing code review but this was marked as resolved. Was this addressed somewhere?

sbesson

The last changes were tested with the following test script

from omero.gateway import BlitzGateway
import omero

print("Connect with user/password, no secure argument")
with BlitzGateway(username=user, passwd=password, host=host) as conn:
    print("Connected: " + str(conn.connect()))
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    print("Creating encrypted client")
    conn.setSecure(True)
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")

print("\n")
print("Connect with user/password and secure=False")
with BlitzGateway(username=user, passwd=password, host=host, secure=False) as conn:
    print("Connected: " + str(conn.connect()))
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    print("Creating encrypted client")
    conn.setSecure(True)
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")

print("\n")
print("Connect with user/password and secure=True")
with BlitzGateway(username=user, passwd=password, host=host, secure=True) as conn:
    print("Connected: " + str(conn.connect()))
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    print("Creating unencrypted client")
    conn.setSecure(False)
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")

c = omero.client(host)
c.createSession(user, password)

print("\n")
print("Connect with encrypted client object")
with BlitzGateway(client_obj=c) as conn:
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    print("Creating unencrypted client")
    conn.setSecure(False)
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    c.closeSession()

c = omero.client(host)
c.createSession(user, password)
c2 = c.createClient(secure=False)
c.__del__()

print("\n")
print("Connect with unencrypted client object")
with BlitzGateway(client_obj=c2) as conn:
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    print("Creating encrypted client")
    conn.setSecure(True)
    print(f"conn.c.isSecure: {conn.c.isSecure()} "
          f"conn.isSecure: {conn.isSecure()} "
          f"conn.secure: {conn.secure}")
    c2.closeSession()

print("\n")
print("Connect with encrypted client object and secure=False")
c = omero.client(host)
c.createSession(user, password)
try:
    conn = BlitzGateway(client_obj=c, secure=False)
except Exception as e:
    print(str(e))
    c.closeSession()


c = omero.client(host)
c.createSession(user, password)
c2 = c.createClient(secure=False)
c.__del__()
print("Connect with unencrypted client object and secure=True")

try:
    conn = BlitzGateway(client_obj=c2, secure=True)
except Exception as e:
    print(str(e))
    c2.closeSession()

with the following output

Connect with user/password, no secure argument
Connected: True
conn.c.isSecure: False conn.isSecure: False conn.secure: False
Creating encrypted client
conn.c.isSecure: True conn.isSecure: True conn.secure: True


Connect with user/password and secure=False
Connected: True
conn.c.isSecure: False conn.isSecure: False conn.secure: False
Creating encrypted client
conn.c.isSecure: True conn.isSecure: True conn.secure: True


Connect with user/password and secure=True
Connected: True
conn.c.isSecure: True conn.isSecure: True conn.secure: True
Creating unencrypted client
conn.c.isSecure: False conn.isSecure: False conn.secure: False


Connect with encrypted client object
conn.c.isSecure: True conn.isSecure: True conn.secure: True
Creating unencrypted client
conn.c.isSecure: False conn.isSecure: False conn.secure: False


Connect with unencrypted client object
conn.c.isSecure: False conn.isSecure: False conn.secure: False
Creating encrypted client
conn.c.isSecure: True conn.isSecure: True conn.secure: True


Connect with encrypted client object and secure=False
Secure flag False and True do not match
Connect with unencrypted client object and secure=True
Secure flag True and False do not match

The state of the secure keyword is now consistent across all usages. Happy fo this to get merged assuming the integration tests pass

jburel · 2024-04-12T13:54:51Z

Integration tests are green but I could add a few more to match your detailed review

jburel · 2024-04-24T08:12:35Z

Supporting tests are now all green. Proposing to merge and release as 5.19.2 (alongside a few approved PRs)

sbesson · 2024-04-24T08:16:39Z

Supporting tests are now all green. Proposing to merge and release as 5.19.2 (alongside a few approved PRs)

No objection from my side. Do you have a shortlist of all PRs proposed for inclusion?

jburel · 2024-04-24T08:20:29Z

short list:
#394
#398
#406
#405 (potentially but in that case tag will be 5.20.0)

will-moore · 2024-04-24T08:25:12Z

Would be great to get all those PRs in 👍

sbesson · 2024-04-24T08:30:31Z

Looks good to me. I am also in favor of including #405 as it reduces the possibility of calls which can cause memory issues server-side. As discussed on the PR, the public API is not modified so I think it could be amenable for either a patch or minor release increment.

jburel requested a review from sbesson February 29, 2024 15:13

jburel mentioned this pull request Feb 29, 2024

[BUG] ezomero for importing error by using omero-user-token for omero connection TheJacksonLaboratory/ezomero#100

Closed

sbesson reviewed Mar 1, 2024

View reviewed changes

jburel added 3 commits March 1, 2024 21:32

set host and port from client if available

9ae6018

set session from client

dbf00ab

do not close session associated to client

72d735c

jburel force-pushed the review_session branch from 12b36f3 to 72d735c Compare March 1, 2024 21:49

remove uncessary check

ee24282

sbesson reviewed Mar 4, 2024

View reviewed changes

jburel changed the title ~~GetSession~~ Session ID and constructor Mar 4, 2024

DavidStirling reviewed Mar 4, 2024

View reviewed changes

will-moore added the exclude label Mar 4, 2024

jburel added 4 commits March 4, 2024 22:27

raise exception if hosts and ports do not match

f30d62a

fix syntax

65f9d94

set the parameters explicitly

150803b

set secure flag from client

489c78b

jburel force-pushed the review_session branch from 8ac6865 to 489c78b Compare March 6, 2024 03:36

return correct flag using isSecure

dd97de9

jburel removed the exclude label Mar 25, 2024

set session ID only

704e50f

reset sessionUuid

e2025ed

joshmoore reviewed Apr 2, 2024

View reviewed changes

jburel added 2 commits April 2, 2024 10:16

return secure flag

88d2453

check if it is the same session

a91f391

jburel mentioned this pull request Apr 2, 2024

Session creation ome/openmicroscopy#6381

Merged

DavidStirling reviewed Apr 6, 2024

View reviewed changes

src/omero/gateway/__init__.py Outdated Show resolved Hide resolved

remove comment

4dc40d6

sbesson reviewed Apr 11, 2024

View reviewed changes

jburel added 4 commits April 11, 2024 11:52

remove conditional

e989c33

review how secure flag is set

2474872

revert to original implementation

01fc825

check compatibility of secure flag

64bf1f9

sbesson approved these changes Apr 12, 2024

View reviewed changes

jburel merged commit 6dde706 into ome:master Apr 24, 2024
9 checks passed

jburel deleted the review_session branch April 29, 2024 13:38

will-moore mentioned this pull request Jun 18, 2024

Reopening BlitzGateway returns False #413

Closed

barrettMCW mentioned this pull request Jun 18, 2024

Fix reconnection issue #415

Closed

Session ID and constructor #400

Session ID and constructor #400

Conversation

jburel commented Feb 29, 2024 • edited Loading

sbesson left a comment • edited Loading

Choose a reason for hiding this comment

jburel commented Mar 1, 2024

jburel commented Mar 1, 2024 • edited Loading

DavidStirling commented Mar 1, 2024

jburel commented Mar 1, 2024 • edited Loading

jburel commented Mar 1, 2024 • edited Loading

jburel commented Mar 1, 2024 • edited Loading

jburel commented Mar 1, 2024

sbesson left a comment

Choose a reason for hiding this comment

jburel commented Mar 4, 2024 • edited Loading

jburel commented Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

jburel Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jburel Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jburel Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

jburel Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

will-moore commented Mar 4, 2024

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jburel commented Apr 5, 2024 • edited Loading

DavidStirling commented Apr 11, 2024

sbesson Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

jburel Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson left a comment

Choose a reason for hiding this comment

jburel commented Apr 12, 2024

jburel commented Apr 24, 2024

sbesson commented Apr 24, 2024

jburel commented Apr 24, 2024

will-moore commented Apr 24, 2024

sbesson commented Apr 24, 2024

jburel commented Feb 29, 2024 •

edited

Loading

sbesson left a comment •

edited

Loading

jburel commented Mar 1, 2024 •

edited

Loading

jburel commented Mar 1, 2024 •

edited

Loading

jburel commented Mar 1, 2024 •

edited

Loading

jburel commented Mar 1, 2024 •

edited

Loading

jburel commented Mar 4, 2024 •

edited

Loading

jburel commented Mar 4, 2024 •

edited

Loading

jburel Mar 4, 2024 •

edited

Loading

jburel Mar 4, 2024 •

edited

Loading

jburel Mar 5, 2024 •

edited

Loading

jburel Mar 6, 2024 •

edited

Loading

jburel commented Apr 5, 2024 •

edited

Loading

sbesson Apr 11, 2024 •

edited

Loading

sbesson Apr 11, 2024 •

edited

Loading

jburel Apr 11, 2024 •

edited

Loading